common/crypto/keccak: vendor in golang.org/x/crypto/sha3#18947
common/crypto/keccak: vendor in golang.org/x/crypto/sha3#18947Sahil-4555 wants to merge 12 commits intoerigontech:mainfrom
Conversation
|
@AskAlexSharov |
|
@Sahil-4555 thank you for picking it up. Will check |
|
@Sahil-4555 Thank you very much for this work! However, we'll probably go with https://github.com/Giulio2002/fastkeccak. Closing in favour of PR #19092. |
ac7bb03 to
9d0cd99
Compare
yperbasis
left a comment
There was a problem hiding this comment.
keccakf_arm64.s doesn't compile on macos: https://github.com/erigontech/erigon/actions/runs/21910296812/job/63266404162
9d0cd99 to
eced6d7
Compare
eced6d7 to
dec569b
Compare
|
@chfast The fastkeccak implementation by Giulio now includes an ARM version which i have added in this PR in addition to the existing AMD64 one. From bechmark observations, Giulio’s implementation appears to have zero allocations, whereas the current version (based on Go’s official crypto/sha3) results in ~32B/op. In terms of performance, I didn’t observe any consistent or significant speed difference between the two implementations in benchmarks. Based on the feedback here, we can move forward with the preferred approach. If this PR is no longer needed, I’m happy to close it. Would appreciate your thoughts. |
4f186a1 to
74b5b43
Compare
There was a problem hiding this comment.
Pull request overview
This PR vendors a local common/crypto/keccak implementation (based on golang.org/x/crypto/sha3 prior to the asm removal) and rewires Erigon’s “legacy Keccak” call sites to use it, aiming to avoid hashing hot-path performance regressions.
Changes:
- Added a vendored
common/crypto/keccakpackage (including amd64/arm64 optimized permutations and KAT tests). - Replaced
golang.org/x/crypto/sha3legacy Keccak usage withcommon/crypto/keccakacross networking, execution, commitment/trie, DB, and txnprovider code. - Updated benchmarks/tests that instantiate legacy Keccak hashers to use the vendored package.
Reviewed changes
Copilot reviewed 33 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| txnprovider/txpool/pool_txn_parser.go | Switch txn parsing contexts to vendored legacy Keccak instances. |
| txnprovider/shutter/internal/crypto/hash.go | Switch shutter keccak256 helper to vendored legacy Keccak. |
| polygon/bor/bor.go | Use vendored legacy Keccak for merkle tree hashing. |
| p2p/rlpx/rlpx.go | Use vendored legacy Keccak for RLPx MAC setup. |
| p2p/enode/idscheme.go | Use vendored legacy Keccak for ENR signing/verification hashes. |
| p2p/dnsdisc/tree.go | Use vendored legacy Keccak for DNS discovery entry hashing/signature hashes. |
| node/shards/state_cache_test.go | Switch test hasher construction to vendored legacy Keccak. |
| execution/vm/instructions.go | Use vendored legacy Keccak for EVM KECCAK256 opcode hasher initialization. |
| execution/tests/testutil/state_test_util.go | Use vendored legacy Keccak for RLP hashing in test utilities. |
| execution/rlp/rlp_test.go | Use vendored legacy Keccak for hashing benchmarks. |
| execution/protocol/rules/ethash/rules.go | Use vendored legacy Keccak for Ethash seal hash. |
| execution/protocol/rules/ethash/algorithm.go | Use vendored legacy Keccak{256,512} for Ethash seed/cache/dataset hashing. |
| execution/protocol/block_exec.go | Use vendored legacy Keccak for RLP hash during block execution. |
| execution/protocol/aa/validation_rules_tracer.go | Use vendored legacy Keccak for associated storage hashing in tracer logic. |
| execution/commitment/trie/hasher.go | Update hasher pool to vendored legacy Keccak while keeping crypto.KeccakState assertions. |
| execution/commitment/trie/hashbuilder.go | Update HashBuilder to vendored legacy Keccak state. |
| execution/commitment/trie/account_node_test.go | Update test hashing to vendored legacy Keccak state. |
| execution/commitment/patricia_state_mock_test.go | Update mock state hashing to vendored legacy Keccak. |
| execution/commitment/hex_patricia_hashed.go | Use vendored legacy Keccak for Patricia-hashed trie internals. |
| execution/commitment/commitment.go | Use vendored legacy Keccak in commitment validation path. |
| db/rawdb/accessors_chain_test.go | Use vendored legacy Keccak in rawdb storage tests. |
| db/kv/kvcache/cache.go | Use vendored legacy Keccak in cache hasher initialization. |
| common/hasher.go | Switch common hasher pool to vendored legacy Keccak. |
| common/crypto/keccak/sha3_test.go | Adds upstream-derived KATs/benchmarks for the vendored Keccak implementation. |
| common/crypto/keccak/sha3.go | Adds upstream-derived sponge/hash implementation used by legacy Keccak. |
| common/crypto/keccak/keccakf_arm64.go | Declares arm64 permutation hook (asm-backed) under build tags. |
| common/crypto/keccak/keccakf_amd64.s | Adds amd64 assembly permutation for keccak-f1600. |
| common/crypto/keccak/keccakf_amd64.go | Declares amd64 permutation hook (asm-backed) under build tags. |
| common/crypto/keccak/keccakf.go | Adds pure-Go permutation for non-asm builds/architectures. |
| common/crypto/keccak/hashes.go | Adds constructors for legacy Keccak-256/512. |
| common/crypto/keccak/README.md | Documents why the vendored Keccak exists and why stdlib SHA-3 can’t be used. |
| common/crypto/keccak/LICENSE | Adds upstream license for vendored code. |
| common/crypto/crypto.go | Switch core crypto helpers/pools to vendored legacy Keccak. |
| common/address.go | Switch checksum hashing to vendored legacy Keccak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sc.SetStorageWrite(a2.Bytes(), 1, l3.Bytes(), nil) | ||
| lastK = lastK[:0] | ||
| if err := sc.WalkStorage(common.BytesToHash(sha3.NewLegacyKeccak256().Sum(a1.Bytes())), 1, nil, func(locHash common.Hash, val []byte) error { | ||
| if err := sc.WalkStorage(common.BytesToHash(keccak.NewLegacyKeccak256().Sum(a1.Bytes())), 1, nil, func(locHash common.Hash, val []byte) error { |
There was a problem hiding this comment.
hash.Hash.Sum does not hash the provided slice; it appends the current digest to the argument. As written, this computes a1.Bytes() || keccak256(empty) instead of keccak256(a1.Bytes()), so WalkStorage will likely not visit the intended entries and the test won't actually validate ordering. Compute the address hash by writing a1.Bytes() into the hasher (or use an existing helper like crypto.Keccak256Hash).
| if err := sc.WalkStorage(common.BytesToHash(keccak.NewLegacyKeccak256().Sum(a1.Bytes())), 1, nil, func(locHash common.Hash, val []byte) error { | |
| hasher := keccak.NewLegacyKeccak256() | |
| if _, err := hasher.Write(a1.Bytes()); err != nil { | |
| t.Fatalf("failed to hash address: %v", err) | |
| } | |
| addrHash := common.BytesToHash(hasher.Sum(nil)) | |
| if err := sc.WalkStorage(addrHash, 1, nil, func(locHash common.Hash, val []byte) error { |
|
closing in favor of mine |
The upstream
golang.org/x/crypto/sha3package has removed the assembly-optimizedKeccakimplementation. To avoid a performance regression in Erigon’s hashing hot paths, we maintain a local Keccak implementation that preserves the amd64 assembly code instead of relying ongolang.org/x/crypto/sha3.Refrence: ethereum/go-ethereum#33323
And: golang/go#75486
--
To verify that keccakf.go, keccakf_amd64.go, keccakf_amd64.s, and sha3.go are identical matches to golang.org/x/crypto@v0.43.0, the version right before the asm was removed.
The hashes.go and sha3_test.go just consists of deletions of unrelated sha3 code.
You can check that the asm optimization is still working by running:
And all internal uses were successfully ported to our vendored packaged.